-
Notifications
You must be signed in to change notification settings - Fork 17
feat: use shiki for syntax highlight #3052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| builtinFunctions, | ||
| keywords, | ||
| typeKeywords, | ||
| } from 'monaco-yql-languages/build/yql/yql.keywords'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constants are not valid yet, and it won't be undated in future. Soon it will be removed from the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates the syntax highlighting implementation from react-syntax-highlighter to shiki. The change improves performance and maintainability by:
- Replacing Prism-based highlighting with the modern Shiki library
- Implementing custom YQL themes based on Monaco VS themes
- Using lazy loading for languages and themes with a singleton highlighter instance
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/SyntaxHighlighter/YDBSyntaxHighlighter.tsx | Replaced react-syntax-highlighter component with async shiki-based highlighting |
| src/components/SyntaxHighlighter/YDBSyntaxHighlighter.scss | Added new content wrapper styles for shiki-generated HTML |
| src/components/SyntaxHighlighter/shikiHighlighter.ts | New module implementing lazy-loaded shiki highlighter with caching |
| src/components/SyntaxHighlighter/themes.ts | Replaced Prism themes with custom TextMate theme registrations for YQL |
| src/components/SyntaxHighlighter/types.ts | Updated to use shiki's BundledLanguage/BundledTheme types |
| src/components/SyntaxHighlighter/yql.ts | Removed Prism YQL language definition (replaced by TextMate grammar) |
| src/components/TruncatedQuery/TruncatedQuery.tsx | Added className prop to YDBSyntaxHighlighter |
| src/components/TruncatedQuery/TruncatedQuery.scss | Added padding override for pre elements |
| package.json | Removed react-syntax-highlighter dependencies, added shiki 3.15.0 |
| package-lock.json | Updated dependency tree for shiki and its transitive dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9 files reviewed, 1 comment
This is needed to unify yql syntax highlight. For now it will use textmate grammar from monaco-yql-languages, which is generated by yql backend team.
Stand
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔺
Current: 66.08 MB | Main: 47.46 MB
Diff: +18.62 MB (39.23%)
ℹ️ CI Information
shikiHighlighter.tswith singleton pattern, lazy loading, and efficient caching of languages/themesYDBSyntaxHighlighternow uses async rendering with loading states and proper cleanupreact-syntax-highlighter(and its types) withshiki3.15.0Implementation Quality
The implementation follows good patterns:
Notable Observations
textchange which may cause frequent re-highlighting in interactive scenariosConfidence Score: 5/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant Component as YDBSyntaxHighlighter participant Hook as useEffect participant Highlighter as shikiHighlighter participant Shiki as Shiki Library participant Cache as Language/Theme Cache Component->>Hook: Mount with text, language, themeType Hook->>Highlighter: highlightCode(text, language, themeType) alt First use of Shiki Highlighter->>Shiki: Dynamic import('shiki') Shiki-->>Highlighter: createHighlighter instance Note over Highlighter: Cache highlighter promise end Highlighter->>Cache: Check if language loaded alt Language not in cache Highlighter->>Shiki: loadLanguage(yql or bundled) Shiki-->>Highlighter: Language loaded Highlighter->>Cache: Add to loadedLanguages end Highlighter->>Cache: Check if theme loaded alt Theme not in cache Highlighter->>Shiki: loadTheme(custom or bundled) Shiki-->>Highlighter: Theme loaded Highlighter->>Cache: Add to loadedThemes end Highlighter->>Shiki: codeToHtml(code, lang, theme) Shiki-->>Highlighter: HTML string with syntax highlighting Highlighter-->>Hook: Return HTML Hook->>Component: setHighlightedHtml(html) Component->>Component: Render with dangerouslySetInnerHTML Note over Component: On text/language/theme change Component->>Hook: Re-run effect Hook->>Highlighter: highlightCode (cached instances) Note over Cache: Reuses cached language/theme Highlighter-->>Component: New highlighted HTML